-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added IgnoredMethods
configuration to Style/CombinableLoops
#8852
Added IgnoredMethods
configuration to Style/CombinableLoops
#8852
Conversation
defa89b
to
1a0c1b3
Compare
Now that #8851 is merged, your example no longer works (methods have different arguments)... |
@marcandre oops, thanks! I had changed the test but not the example, I’ll fix it. |
1a0c1b3
to
01ffcce
Compare
@marcandre updated! |
Cool. Now why should these two |
@marcandre definitely true that you could. I was trying to come up with an example of something you might not want to combine despite being able to, this is maybe not the best example though. Overall for this I was just thinking based on the original issue that there is a use case to make this cop less restrictive without having to be manually disabled (because the cop just looks for a method that starts or ends with |
The patch is well written and not particularly complex, but I'd wait until
we have someone providing an actual example of false positive.
For example, had it been merged originally, we might not have had the
report about different arguments...
…On Mon, Oct 5, 2020, 07:54 Daniel Vandersluis ***@***.***> wrote:
@marcandre <https://github.com/marcandre> definitely true that you could.
I was trying to come up with an example of something you might not want to
combine despite being able to, this is maybe not the best example though.
Overall for this I was just thinking based on the original issue that
there is a use case to make this cop less restrictive without having to be
manually disabled (because the cop just looks for a method that starts or
ends with each but it’s conceivable that there are methods that match
that aren’t enumerators).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8852 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAIH2QR562HA5A4S2ODOZLSJGXWHANCNFSM4SDNANSA>
.
|
To be fair, we didn't have the report about different arguments, I discovered it while trying to replicate the original issue 😁. This PR was my first attempt at fixing it (since the example provided in the issue wasn't actually failing) which I split off once I discovered that there actually was a legit issue that could be fixed (the arguments). I figured it could still be valuable so I split it out of my original branch and put it up by itself so we could have this exact discussion. I'm ok with closing this though if you don't think it's necessary! |
01ffcce
to
1e66f43
Compare
I'm leaning towards closing the PR at this point, as I'm not sure it solves an actual problem. We can revisit it down the road if the need to do so arises. |
Related to #8848, it probably makes sense to make
Style/CombinableLoops
useIgnoredMethods
, since it's possible to have a method that matches the method name conditions without being combinable.Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and RuboCop for itself, and generates the documentation.